Skip to content

Optimisation des requêtes pour l'économie#1333

Draft
gtolontop wants to merge 9 commits into
ServerOpenMC:masterfrom
gtolontop:fix/economy-balance-save
Draft

Optimisation des requêtes pour l'économie#1333
gtolontop wants to merge 9 commits into
ServerOpenMC:masterfrom
gtolontop:fix/economy-balance-save

Conversation

@gtolontop

@gtolontop gtolontop commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Petit résumé de la PR:

Optimise la persistance des soldes économie en regroupant les écritures DB des balances modifiées.

Étape nécessaire afin que la PR soit fini (si PR en draft)

  • Suivre le Code de Conduite
  • Enlever tous les imports non utilisés
  • Bien documenter la feature
  • Fournir un profileur (non demandé pour cette PR)
  • Avoir une milestone associée à la PR (2.5.0 à assigner)
  • Valider tout les checks
  • Tester et valider la feature/changement

Decrivez vos changements

EconomyManager écrivait en DB à chaque changement de solde via playersDao.createOrUpdate, notamment depuis setBalance, addBalance et withdrawBalance.

Cette PR garde les soldes à jour en mémoire immédiatement, marque uniquement les comptes modifiés comme à sauvegarder, puis persiste ces comptes en batch avec saveAllBalances() :

  • à l'arrêt de la feature, via le cycle de sauvegarde existant ;
  • périodiquement via un autosave, pour limiter la perte possible en cas de crash.

Le cache utilise aussi computeIfAbsent pour conserver les nouveaux comptes en mémoire avant leur première sauvegarde. En cas d'erreur pendant la sauvegarde batch, les comptes concernés sont remis dans la liste à sauvegarder.

@gtolontop gtolontop changed the title [codex] Batch economy balance persistence Optimise economy balance persistence Jun 22, 2026
@gtolontop gtolontop force-pushed the fix/economy-balance-save branch from 6c8e2c3 to 70dde0d Compare June 22, 2026 16:30
@gtolontop gtolontop force-pushed the fix/economy-balance-save branch from 70dde0d to a2a7db9 Compare June 22, 2026 16:32
@AxenoDev

Copy link
Copy Markdown
Member

au lieu de sauvegarder tout les x temps, pourquoi tu save pas juste a la db au moment ou le plugin se desactive

@iambibi

iambibi commented Jun 22, 2026

Copy link
Copy Markdown
Member

C'était marqué dans l'issue

@gtolontop

Copy link
Copy Markdown
Contributor Author

au lieu de sauvegarder tout les x temps, pourquoi tu save pas juste a la db au moment ou le plugin se desactive

C’est déjà fait xd au save() de la feature, donc à la désactivation du plugin, saveAllBalances() flush les balances modifiées

L’autosave est volontaire en plus du shutdown save ça évite de perdre toutes les modifications depuis le démarrage si le serveur crash/kill ou si l’arrêt ne va pas jusqu’au bout. Et il ne sauvegarde pas tous les balances à chaque fois, seulement les UUID marqués dirty depuis la dernière sauvegarde :)

@AxenoDev

AxenoDev commented Jun 22, 2026

Copy link
Copy Markdown
Member

Le serveur n'est pas cense etre kill ou crash, meme il ne sera jamais kill...

@iambibi

iambibi commented Jun 22, 2026

Copy link
Copy Markdown
Member

Ben après il peut crash oui. Mais il est tjr éteint à 2h donc cv

@gtolontop

Copy link
Copy Markdown
Contributor Author

Même si le serveur n’est pas censé crash il peut crash mdrrrrr l’autosave limite la perte si ça arrive et dans tous les cas le coût en perf est faible on ne save pas tous les comptes seulement les dirty en batch

@iambibi

iambibi commented Jun 22, 2026

Copy link
Copy Markdown
Member

Tu appelles quoi un compte dirty?

@gtolontop

Copy link
Copy Markdown
Contributor Author

Dirty = modifié depuis le dernier save DB
On garde l’UUID dans dirtyBalances et au prochain save on persiste seulement ces comptes

@AxenoDev

Copy link
Copy Markdown
Member

Le probleme, est que la le lag a lieu lors de la premiere connexion des joeurs, c'est un moment que l'on avait fait avec 50bots de souvenir, et le lag venait du save a la db, donc la si je comprends bien, a un moment, le serveur va lag parcequ'il save toutes les donne dans la db a un moment x

@iambibi

iambibi commented Jun 22, 2026

Copy link
Copy Markdown
Member

Fin et déjà pour qu'on valide le problème, il faudra que tu donnes un jar de ton plugin, on fait un teste avec 50-100 joueurs, et on attends jusqu'au moment où le scheduler s'exécute pour save la db, et on tentera

@AxenoDev

Copy link
Copy Markdown
Member

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

@Shoccapik

Copy link
Copy Markdown

Le satan qui se réveille avec le full vibecoding qui fait mal

@gtolontop

Copy link
Copy Markdown
Contributor Author

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

MDRRRRR, elle est vraiment niquel ma PR je vois pas le soucis + c'est pas du vibecoding mais j'avoue que mon code a une vibe très propre :)

Ta mal pris le faite que tu es tort ici ?
image
ou ici ?
image

@gtolontop

Copy link
Copy Markdown
Contributor Author

Le satan qui se réveille avec le full vibecoding qui fait mal

Très propre se vibecoding en tous cas

@gtolontop

Copy link
Copy Markdown
Contributor Author

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

Dis-moi, qu'aurais-tu fait différemment ?

@AxenoDev

AxenoDev commented Jun 23, 2026

Copy link
Copy Markdown
Member

Donc pourquoi il y a ecris CODEX dans ta PR ....
Ne me prends pas pour un con non plus

@AxenoDev

Copy link
Copy Markdown
Member

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

Dis-moi, qu'aurais-tu fait différemment ?

Honnetement, bcp de chose, mais si tu "ne vibecode pas" chaque developpeur fait differemment donc je n'aurais pas fait pareil que toi

@iambibi

iambibi commented Jun 23, 2026

Copy link
Copy Markdown
Member

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

MDRRRRR, elle est vraiment niquel ma PR je vois pas le soucis + c'est pas du vibecoding mais j'avoue que mon code a une vibe très propre :)

Ta mal pris le faite que tu es tort ici ?
image
ou ici ?
image

Tu mets tout sur le principe de la raison? Pourquoi ?
Tu as des choses à montrer?
Avoir tort n'est pas négatif le temps que la personne explique pourquoi elle a tort.

@iambibi

iambibi commented Jun 23, 2026

Copy link
Copy Markdown
Member

Fin je dis pas ton initiative est bonne mais quand on voit le préfixe [codex], on a tout de suite moins envie de la merge, de plus tu as une très bonne connaissance du plugin. Tellement une bonne connaissance que tu ajoutes même des tests, des appels à OMCLogger (qui est totalement nouveau et que je doute que tu l'as vu sans une IA qui a codé). Fabuleux !

@AxenoDev

Copy link
Copy Markdown
Member

Bon @gtolontop, si c'est pour faire du vibecoding sur le projet ce n'est pas la peine de venir faire une PR.

MDRRRRR, elle est vraiment niquel ma PR je vois pas le soucis + c'est pas du vibecoding mais j'avoue que mon code a une vibe très propre :)

Ta mal pris le faite que tu es tort ici ? image ou ici ? image

image Voila a quoi je penses quand je vois ton message

@gtolontop

Copy link
Copy Markdown
Contributor Author

J’ai l’impression de me faire clasher alors que ma PR, en soi, je la trouve vraiment bonne. J’ai réellement passé du temps dessus, j’ai réellement codé et testé les choses.

Déjà my bad pour le message au-dessus, je l’ai mal pris qu’on me dise ça sans même vraiment review mon travail. Là j’ai surtout l’impression que la discussion part sur “Codex / vibecoding” au lieu de parler du code de la PR et du travail que j’ai vraiment fait.

Je n’ai jamais caché avoir utilisé Codex. C’est un très bon outil, je l’ai utilisé notamment pour m’aider à relire, review, trouver des points faibles et améliorer ma PR. Je ne vois pas le problème en soi tant que le code est relu, compris, testé, et corrigé proprement derrière.

J’ai corrigé le code moi-même et j’ai écrit le code de la PR. Ce n’est pas du vibecoding où j’ai juste donné l’issue et où il a tout fait. Codex m’a surtout servi de review et d’analyse. Par exemple, quand j’ai voulu finaliser proprement avec des tests et une meilleure gestion des erreurs, il m’a aidé à voir l’existence de OMCLogger, puis j’ai vérifié et je l’ai utilisé.

Le souci, c’est que j’ai l’impression que la PR est jugée parce qu’il y a écrit Codex dans le tag, sans pointer un vrai problème dans le code. Si le code est mauvais, dites-moi précisément où : logique, perf, API cassée, comportement incorrect, test manquant, etc. Et je corrigerai avec plaisir.

Sur le fond, la PR règle bien l’issue #1332 :

  • avant, on pouvait faire un createOrUpdate DB à chaque mutation de balance ;
  • maintenant la balance est modifiée directement en mémoire ;
  • l’UUID est marqué dirty ;
  • saveAllBalances() persiste ensuite seulement les comptes dirty en batch ;
  • le shutdown save existe toujours ;
  • l’autosave est juste une sécurité en plus, et ne sauvegarde pas toute l’économie à chaque fois.

Un compte dirty, c’est juste un compte modifié depuis le dernier save DB. Donc si 5 comptes ont changé, on save ces 5 comptes, pas toute la table.

Comme je disais pour OMCLogger, oui Codex m’a aidé à voir que ça existait, et après j’ai vérifié et je l’ai utilisé. Je ne vois pas en quoi c’est un problème : c’est justement mieux d’utiliser l’outil déjà présent dans le projet plutôt qu’un log random ou rien du tout.

J’ai testé IG, pas seulement envoyé du code au hasard. Les tests unitaires liés à MockBukkit ont des échecs connus à cause du setup global du plugin / Dream / Contest / NMS, mais les tests ajoutés sur la logique economy passent.

Donc vraiment, si vous ne voulez pas merge la PR, donnez-moi une raison technique concrète. Si y’a un vrai souci dans la méthode ou dans le code, je le corrige. Mais juste dire “vibecoding” parce qu’il y a Codex dans le process et dans le tag, sans pointer ce qui est faux dans la PR, ça ne m’aide pas à améliorer quoi que ce soit.

@iambibi

iambibi commented Jun 24, 2026

Copy link
Copy Markdown
Member

On va réfléchir à un débat entre le staff.
Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?.
Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

@ltuffery

Copy link
Copy Markdown
Contributor

Bonjour, la PR est-elle prête à être revue ? Je vois qu’elle est encore en draft.

  1. Concernant les tests, tu utilises de la réflexion. Ce n’est généralement pas recommandé. Je ne sais pas si Mockito est inclus dans le projet, mais si c’est le cas, ce serait préférable de l’utiliser et de supprimer la réflexion.

  2. Est-il possible d’en profiter pour corriger les tests unitaires de l’Economy Manager ? Certains sont actuellement en échec, et comme tu as une PR ouverte dessus, ce serait intéressant de les stabiliser.

  3. J’ai également remarqué plusieurs copies en mémoire. Pourquoi ne pas modifier directement l’objet déjà instancié ?

@ltuffery ltuffery assigned ltuffery and gtolontop and unassigned ltuffery Jun 25, 2026
@ltuffery ltuffery added 🔄 Changement Un petit changement 🆙 Amélioration Une amélioration sur quelque chose labels Jun 25, 2026
@iambibi iambibi added the ⚡Optimisation Une amélioration niveau performance label Jun 25, 2026
@iambibi iambibi added this to the 2.5.0-beta-1 milestone Jun 25, 2026
@gtolontop

Copy link
Copy Markdown
Contributor Author

On va réfléchir à un débat entre le staff. Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?. Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

Oui j’ai résumé trop vite xd

sur les tests j'obtiens

  • EconomyManagerDirtySaveTest : 2 tests, 0 fail
  • EconomyManagerTest : 16 tests, 5 fail

Dans le XML de test, les fails sont :

  • testSuccessWithdrawBalance() -> DreamSteps fail à l’init parce que DreamDimensionManager.DREAM_WORLD est null.
  • les 4 tests avec transactions/reason -> NoClassDefFoundError: net/minecraft/network/protocol/Packet, déclenché via ContestManager.initPhase3.

Donc quand j’ai dit Dream/NMS, je parlais de ces causes-là, mais j'essaierais d'être plus précis directement les prochaines fois

@iambibi

iambibi commented Jun 25, 2026

Copy link
Copy Markdown
Member

On va réfléchir à un débat entre le staff. Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?. Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

Oui j’ai résumé trop vite xd

sur les tests j'obtiens

  • EconomyManagerDirtySaveTest : 2 tests, 0 fail
  • EconomyManagerTest : 16 tests, 5 fail

Dans le XML de test, les fails sont :

  • testSuccessWithdrawBalance() -> DreamSteps fail à l’init parce que DreamDimensionManager.DREAM_WORLD est null.
  • les 4 tests avec transactions/reason -> NoClassDefFoundError: net/minecraft/network/protocol/Packet, déclenché via ContestManager.initPhase3.

Donc quand j’ai dit Dream/NMS, je parlais de ces causes-là, mais j'essaierais d'être plus précis directement les prochaines fois

Tkt je préfère que tu me demandes des choses comme cela a la place de l'IA qui te résume mais qui sait pas vrm 👍

@iambibi

iambibi commented Jun 25, 2026

Copy link
Copy Markdown
Member

On va réfléchir à un débat entre le staff. Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?. Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

Oui j’ai résumé trop vite xd

sur les tests j'obtiens

  • EconomyManagerDirtySaveTest : 2 tests, 0 fail
  • EconomyManagerTest : 16 tests, 5 fail

Dans le XML de test, les fails sont :

  • testSuccessWithdrawBalance() -> DreamSteps fail à l’init parce que DreamDimensionManager.DREAM_WORLD est null.
  • les 4 tests avec transactions/reason -> NoClassDefFoundError: net/minecraft/network/protocol/Packet, déclenché via ContestManager.initPhase3.

Donc quand j’ai dit Dream/NMS, je parlais de ces causes-là, mais j'essaierais d'être plus précis directement les prochaines fois

Tu vois je savais pas que ça crash a cause de la dimension des rêves j'avais pas check et le problème c'est les transactions en async qui fail tt

@gtolontop

Copy link
Copy Markdown
Contributor Author

Bonjour, la PR est-elle prête à être revue ? Je vois qu’elle est encore en draft.

  1. Concernant les tests, tu utilises de la réflexion. Ce n’est généralement pas recommandé. Je ne sais pas si Mockito est inclus dans le projet, mais si c’est le cas, ce serait préférable de l’utiliser et de supprimer la réflexion.
  2. Est-il possible d’en profiter pour corriger les tests unitaires de l’Economy Manager ? Certains sont actuellement en échec, et comme tu as une PR ouverte dessus, ce serait intéressant de les stabiliser.
  3. J’ai également remarqué plusieurs copies en mémoire. Pourquoi ne pas modifier directement l’objet déjà instancié ?

Bonjour, elle est encore en draft justement parce que je préfère traiter les retours avant de la passer ready.

Pour la réflexion dans les tests je comprends se que tu dis mais j’ai vérifié, Mockito n'est pas dans les dépendances du projet. Je peux soit ajouter Mockito si vous êtes ok avec ça, soit refactorer le test / la logique pour éviter la réflexion.

Pour les tests EconomyManagerTest, oui je peux regarder pour les stabiliser. Actuellement, sur mon run local, les nouveaux tests isolés EconomyManagerDirtySaveTest passent, mais EconomyManagerTest a encore 5 fails liés au chargement global du plugin / scheduler lancé pendant MockBukkit.

Pour les copies mémoire elles sont là pour éviter qu’un objet mutable récupéré depuis l’API soit modifié sans passer par le dirty tracking. par exemple si getPlayerBank() retourne l’objet live, un appelant peut faire deposit() dessus sans marquer l’UUID dirty. Les copies évitent ça. Après, si vous préférez une autre approche côté API, je peux adapter.

@gtolontop

Copy link
Copy Markdown
Contributor Author

On va réfléchir à un débat entre le staff. Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?. Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

Oui j’ai résumé trop vite xd
sur les tests j'obtiens

  • EconomyManagerDirtySaveTest : 2 tests, 0 fail
  • EconomyManagerTest : 16 tests, 5 fail

Dans le XML de test, les fails sont :

  • testSuccessWithdrawBalance() -> DreamSteps fail à l’init parce que DreamDimensionManager.DREAM_WORLD est null.
  • les 4 tests avec transactions/reason -> NoClassDefFoundError: net/minecraft/network/protocol/Packet, déclenché via ContestManager.initPhase3.

Donc quand j’ai dit Dream/NMS, je parlais de ces causes-là, mais j'essaierais d'être plus précis directement les prochaines fois

Tkt je préfère que tu me demandes des choses comme cela a la place de l'IA qui te résume mais qui sait pas vrm 👍

J'avais bien run les tests juste j'avais pas pensé a te remonter exactement les tests xd

@gtolontop

Copy link
Copy Markdown
Contributor Author

On va réfléchir à un débat entre le staff. Juste je réponds sur un point, les tests ne fail pas a cause des NMS/Dream???? Tu parles bien de la dimension des rêves ?. Toutes les features utilisant les NMS ne peuvent pas être testée. Et ne sont pas chargée ds les tests.

Oui j’ai résumé trop vite xd
sur les tests j'obtiens

  • EconomyManagerDirtySaveTest : 2 tests, 0 fail
  • EconomyManagerTest : 16 tests, 5 fail

Dans le XML de test, les fails sont :

  • testSuccessWithdrawBalance() -> DreamSteps fail à l’init parce que DreamDimensionManager.DREAM_WORLD est null.
  • les 4 tests avec transactions/reason -> NoClassDefFoundError: net/minecraft/network/protocol/Packet, déclenché via ContestManager.initPhase3.

Donc quand j’ai dit Dream/NMS, je parlais de ces causes-là, mais j'essaierais d'être plus précis directement les prochaines fois

Tu vois je savais pas que ça crash a cause de la dimension des rêves j'avais pas check et le problème c'est les transactions en async qui fail tt

Si tu fais une issue je pourrais regarder :)

@iambibi

iambibi commented Jun 25, 2026

Copy link
Copy Markdown
Member

Si tu fais une issue je pourrais regarder :)

Tu peux la faire aussi, elle sera validé tkt pas

@gtolontop

Copy link
Copy Markdown
Contributor Author

Si tu fais une issue je pourrais regarder :)

Tu peux la faire aussi, elle sera validé tkt pas

Tu veux que je commit sur la même PR ?

@iambibi

iambibi commented Jun 25, 2026

Copy link
Copy Markdown
Member

Généralement, tu dois ouvrir ta PR pour qu'on sache qu'on peut review. Fait comme cela, sinon il risque d'avoir des oublis

@iambibi

iambibi commented Jun 25, 2026

Copy link
Copy Markdown
Member

Si tu fais une issue je pourrais regarder :)

Tu peux la faire aussi, elle sera validé tkt pas

Tu veux que je commit sur la même PR ?

De fixer les tests? Fait en une autre

@iambibi iambibi changed the title Optimise economy balance persistence Optimisation des requêtes pour l'économie Jun 25, 2026
@iambibi

iambibi commented Jun 26, 2026

Copy link
Copy Markdown
Member

Généralement, tu dois ouvrir ta PR pour qu'on sache qu'on peut review. Fait comme cela, sinon il risque d'avoir des oublis

ne t'étonne pas si ta PR ne se fait pas review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆙 Amélioration Une amélioration sur quelque chose 🔄 Changement Un petit changement ⚡Optimisation Une amélioration niveau performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Optimisation de EconomyManager.setBalance

5 participants